Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] feat: playing sound from remote source #282

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tirz
Copy link
Contributor

@tirz tirz commented Mar 31, 2020

WIP

run --example http_flac --features=https-rustls is already working

@tirz tirz force-pushed the feature-sound_via_network branch 3 times, most recently from 134f47b to 84e45b0 Compare March 31, 2020 09:13
Cargo.toml Outdated
@@ -15,6 +16,7 @@ hound = { version = "3.3.1", optional = true }
lazy_static = "1.0.0"
lewton = { version = "0.10", optional = true }
minimp3 = { version = "0.3.2", optional = true }
reqwest = { version = "0.9.0", default-features = false, features = ["rustls-tls"], optional = true }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove the rustls-tls feature from here, copy the reqwest dependency and add it below as a dev-dependency, with rustls-tls enabled? That way, users of rodio have free choice over the tls implementation.

@@ -7,6 +7,7 @@ description = "Audio playback library"
keywords = ["audio", "playback", "gamedev"]
repository = "https://github.com/RustAudio/rodio"
documentation = "http://docs.rs/rodio"
autoexamples = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant as true is already the default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact I am not even sure if setting it to true have any effect...
But at least, it remove a warning.

Since I am setting an example like:

[[example]]
name = "http_flac"
path = "examples/http_flac.rs"
required-features = ["http"]

I got a warning:

warning: An explicit [[example]] section is specified in Cargo.toml which currently
disables Cargo from automatically inferring other example targets.
This inference behavior will change in the Rust 2018 edition and the following
files will be included as a example target:

* /home/tirz/Documents/Rust/rodio/examples/basic.rs
* /home/tirz/Documents/Rust/rodio/examples/music_mp3.rs
* /home/tirz/Documents/Rust/rodio/examples/music_flac.rs
* /home/tirz/Documents/Rust/rodio/examples/reverb.rs
* /home/tirz/Documents/Rust/rodio/examples/music_wav.rs
* /home/tirz/Documents/Rust/rodio/examples/music_ogg.rs
* /home/tirz/Documents/Rust/rodio/examples/spatial.rs

This is likely to break cargo build or cargo test as these files may not be
ready to be compiled as a example target today. You can future-proof yourself
and disable this warning by adding `autoexamples = false` to your [package]
section. You may also move the files to a location where Cargo would not
automatically infer them to be a target, such as in subfolders.

For more information on this warning you can consult
https://github.com/rust-lang/cargo/issues/5330


fn main() {
let device = rodio::default_output_device().unwrap();
let sink = rodio::Sink::new(&device);
let device = rodio::default_output_device().unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rodio uses spaces for indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I planed to run cargo fmt at the end of this PR but right now, I got an error because of my version of Rust.

Warning: Unknown configuration option `fn_args_density`
Warning: Unknown configuration option `fn_brace_style`
Warning: Unknown configuration option `fn_call_style`
Warning: Unknown configuration option `fn_empty_single_line`
Warning: Unknown configuration option `generics_indent`
Warning: Unknown configuration option `impl_empty_single_line`
Warning: Unknown configuration option `reorder_imported_names`
Warning: Unknown configuration option `reorder_imports_in_group`
Warning: Unknown configuration option `where_density`
Warning: Unknown configuration option `where_style`
Warning: Unknown configuration option `wrap_match_arms`
Warning: Unknown configuration option `write_mode`
Error: Decoding config file failed:
unknown variant `Visual`, expected one of `Compressed`, `Tall`, `Vertical` for key `fn_args_layout`
Please check your config file.

I will fix it later.

@tirz tirz force-pushed the feature-sound_via_network branch 2 times, most recently from 4d223b0 to 01b3ce8 Compare April 3, 2020 08:19
@tirz tirz force-pushed the feature-sound_via_network branch from 01b3ce8 to 8f63649 Compare April 3, 2020 08:37
@tirz
Copy link
Contributor Author

tirz commented May 4, 2020

Hey, sorry for my late, I was focused on another project.

In fact this PR is already pretty stable...
Except when the size of the sound is too heavy for our bandwidth.
In that case, the sound may cut until the next block is downloaded, but I do not see a better way to handle it.
Should we wait until N blocks are downloaded? When N may vary with sizeOfFile / downloadSpeed.

Do we need some unit test or just a cargo fmt will be enough?

@est31
Copy link
Member

est31 commented May 4, 2020

I don't really want global cargo fmt to be run that changes all of rodio's source code, especially not in a PR that changes functional things. That way, you can't really inspect the changes any more.

You are already now changing the formatting of files unrelated to the PR.

Can you maybe file a separate PR, find stable alternatives to the rustfmt options present and then run cargo fmt?

Also I'm not 100% convinced that this PR fits in the scope of rodio, as downloading might be done by games rather at the asset layer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants